test: run faster and cleanup after run#8848
Conversation
|
CI again now that a few issues in CI have been cleaned up:https://ci.nodejs.org/job/node-test-commit/5392/ |
indutny
left a comment
indutny
left a comment
There was a problem hiding this comment.
I'm fine with cleaning it up on exit, but I don't see huge benefit from using asynchronous functions here. Is there much point in it?
|
(CI is yellow. First time in a long time. Green can't be too far behind, can it? CAN IT?!?!) |
|
/ping @geek Any response to this from @indutny?:
These test failures have been a long-standing significant problem and I'd really love to get that question resolved so we can land this. Let me know if you'd like me to do anything. EDIT: Oh, these are test improvements but they don't solve the regular test failures. My mistake. |
|
@indutny if you think moving back to sync is better, I can try that... I do think this is easy enough to reason about as it is written. Do you want this to be sync? |
|
New CI here: https://ci.nodejs.org/job/node-test-commit/5440/ If this PR intends to fix #8725 we should put a log in the fire. That bug has been coloring our test environment red for a while.. |
|
@indutny reverted to sync style. The tests are still faster and more reliable! |
|
Looks much better now, thank you! Few questions:
|
|
@indutny first, cleaning up the child processes... I noticed that they would often remain after the tests all ran. After this cleanup, the time between retrying to read the ticks and find the expected value was reduced, which is really where the speedup came from. I will go through the remaining tests and see if I can find similar issues. |
|
Curious if this helps flakiness on the Raspberry Pi devices. Stress test on current master: https://ci.nodejs.org/job/node-stress-single-test/976/nodes=pi2-raspbian-wheezy/console Stress test on this PR: https://ci.nodejs.org/job/node-stress-single-test/977/nodes=pi2-raspbian-wheezy/console |
|
For that matter, let's try the Pi 3 too: master: https://ci.nodejs.org/job/node-stress-single-test/978/nodes=pi3-raspbian-jessie/console this PR: https://ci.nodejs.org/job/node-stress-single-test/979/nodes=pi3-raspbian-jessie/console |
|
@indutny do you approve the PR? |
|
All tests are green, I reran linux-ppc and it passed: https://ci.nodejs.org/job/node-test-commit-plinux/4619/ @indutny after you approve the PR it will be ready for merge. |
|
Approved! LGTM. Thank you |
PR-URL: #8848 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Matthew Loring <mattloring@google.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
|
Landed in b88612d. Thank you! |
|
lts? |
|
Possibly. @indutny what do you think about pulling this back to v4? |
|
I'm fine with backporting it |
PR-URL: #8848 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Matthew Loring <mattloring@google.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
PR-URL: #8848 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Matthew Loring <mattloring@google.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
|
bah... the test doesn't exist in v4.x... changing to do not land |
10 participants
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test
Description of change
Fixes #8725
tick tests now run faster and cleanup spawned processes. Previously, when executed continuously the tick tests would leave spawned processes after the test completed.